Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update to cancel downstream operation in TimeoutStrategy.Pessimistic #1697

Merged
merged 5 commits into from
Oct 17, 2023
Merged

Update to cancel downstream operation in TimeoutStrategy.Pessimistic #1697

merged 5 commits into from
Oct 17, 2023

Conversation

lor1mp
Copy link
Contributor

@lor1mp lor1mp commented Oct 13, 2023 •
edited
Loading

This PR has changes to extend the life of the combinedTokenSource in the AsyncTimeoutEngine, so that the cancellation will get propegated downstream in the case of a TimeoutRejectedException.

Pull Request

The issue or feature being addressed

We noticed that the downstream cancellation token was never signaled in TimeoutStrategy.Pessimistic. The root cause seems to be the TimeoutRejectionException escaping the scope of the ImplementationAsync function on AsyncTimeoutEngine.cs:5. This causes the CancellationTokenSource to be disposed and the downstream token to never get canceled.

This is related to this issue: #722

Details on the issue fix or feature implementation

In this fix we explicitly cancel the combinedTokenSource as the function leaves scope, because without the exception will propagate and cause the source to be disposed before the downstream token can be canceled. This issue was demonstrated in a UT. However, that UT requires that we use "real time" as the TimeSpecBase.cs SystemClock overrides actually cancel the downstream token themselves on a timeout.

Confirm the following

  • I started this PR by branching from the head of the default branch
  • I have targeted the PR to merge into the default branch
  • I have included unit tests for the issue/feature
  • I have successfully run a local build

@lor1mp
Update to extend the lifetime of the combinedTokenSource so that it w…
66b8bdb 
…ill cancel downstream operations in TimeoutStrategy.Pessimistic.
@lor1mp lor1mp changed the title Update to extend the lifetime of the combinedTokenSource so that it w… Update to cancel downstream operation in TimeoutStrategy.Pessimistic Oct 13, 2023
@lor1mp lor1mp mentioned this pull request Oct 13, 2023
Closed
martincostello
martincostello reviewed Oct 15, 2023
View reviewed changes
test/Polly.Specs/Timeout/TimeoutAsyncSpecs.cs Outdated Show resolved Hide resolved
test/Polly.Specs/Timeout/TimeoutAsyncSpecs.cs Outdated Show resolved Hide resolved
src/Polly/Timeout/AsyncTimeoutEngine.cs Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 16, 2023 •
edited
Loading

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (862690e) 84.63% compared to head (9a36f38) 84.65%.
Report is 5 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1697      +/-   ##
==========================================
+ Coverage   84.63%   84.65%   +0.02%     
==========================================
  Files         306      306              
  Lines        6819     6831      +12     
  Branches     1045     1047       +2     
==========================================
+ Hits         5771     5783      +12     
  Misses        839      839              
  Partials      209      209
Flag Coverage Δ
macos 84.65% <85.71%> (+0.02%) ⬆️
windows 84.65% <85.71%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/Polly/Timeout/AsyncTimeoutEngine.cs 93.10% <85.71%> (+0.51%) ⬆️

... and 9 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lor1mp
Copy link
Contributor Author

lor1mp commented Oct 16, 2023

@martincostello - sorry this is my first PR into Polly. I'm not sure how to go about covering this line? (This is the line it's telling me I'm missing right?) Is it saying I don't have the false condition of the branch, and it's just way off?
image

@martincostello
Copy link
Member

Don't worry about the coverage comment, it's more advisory as long as the PR status checks are green.

From looking at it, I think it's impossible to hit anyway.

@martincostello martincostello added this to the v8.1.0 milestone Oct 17, 2023
@martincostello martincostello enabled auto-merge (squash) October 17, 2023 13:41
@lor1mp
Copy link
Contributor Author

lor1mp commented Oct 17, 2023

Thanks for the approval @martincostello. Do I need to do anything about the code-ql failure? Is there anything left for me to do?

@martincostello
Copy link
Member

No, it's just being especially flaky today and I'm retrying it until it passes 😅

@martincostello martincostello merged commit 940c628 into App-vNext:main Oct 17, 2023
18 checks passed
@lor1mp lor1mp deleted the lor1mp/resolve-cancellation-race branch October 17, 2023 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martincostello martincostello martincostello approved these changes

Assignees
No one assigned
Labels
bug fix
Projects
None yet
Milestone
v8.1.0
Development

Successfully merging this pull request may close these issues.
2 participants
@lor1mp @martincostello